Skip to content

Add title to filter schemas#4355

Merged
bajtos merged 1 commit into
loopbackio:masterfrom
dougal83:feat(repository-json-schema)add-title-to-filter-schemas
Jan 23, 2020
Merged

Add title to filter schemas#4355
bajtos merged 1 commit into
loopbackio:masterfrom
dougal83:feat(repository-json-schema)add-title-to-filter-schemas

Conversation

@dougal83

@dougal83 dougal83 commented Jan 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adding titles to json filter schemas as groundwork toward openapi spec ref consolidation based on title property. See #4290

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide

@dougal83
dougal83 requested a review from jannyHou January 1, 2020 20:43
This was referenced Jan 1, 2020
Closed
Closed
bajtos
bajtos suggested changes Jan 7, 2020
View reviewed changes

@bajtos bajtos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @dougal83, I love this idea!

Let's discuss implementation details now.

Comment thread packages/openapi-v3/src/__tests__/unit/filter-schema.unit.ts Outdated
Comment thread packages/repository-json-schema/src/filter-json-schema.ts Outdated
@bajtos bajtos self-assigned this Jan 7, 2020
@bajtos bajtos changed the title Feat(repository json schema)add title to filter schemas Add title to filter schemas Jan 7, 2020
@bajtos bajtos added feature developer-experience Issues affecting ease of use and overall experience of LB users labels Jan 7, 2020
@bajtos bajtos mentioned this pull request Jan 7, 2020
Merged
4 tasks
@dougal83
dougal83 force-pushed the feat(repository-json-schema)add-title-to-filter-schemas branch 2 times, most recently from 78604a9 to 8bfe446 Compare January 8, 2020 23:20
@dougal83

dougal83 commented Jan 8, 2020

Copy link
Copy Markdown
Contributor Author

I've done a git rebase -i to fix history but that is probably a bad idea... not having a good evening.

@dougal83
dougal83 requested a review from bajtos January 9, 2020 00:15
bajtos
bajtos suggested changes Jan 10, 2020
View reviewed changes

@bajtos bajtos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely, the pull request is shaping up nicely 👍

Let's do few more iterations to iron out rough edges.

Comment thread packages/openapi-v3/src/__tests__/unit/filter-schema.unit.ts Outdated
Comment thread packages/openapi-v3/src/filter-schema.ts Outdated
Comment thread packages/openapi-v3/src/filter-schema.ts Outdated
Comment thread packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts Outdated
Comment thread packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts Outdated
Comment thread packages/repository-json-schema/src/filter-json-schema.ts Outdated
Comment thread packages/repository-json-schema/src/filter-json-schema.ts Outdated
Comment thread packages/repository-json-schema/src/filter-json-schema.ts Outdated
Comment thread packages/repository-json-schema/src/filter-json-schema.ts Outdated
Comment thread packages/repository-json-schema/src/filter-json-schema.ts Outdated
@bajtos

bajtos commented Jan 10, 2020

Copy link
Copy Markdown
Member

I've done a git rebase -i to fix history but that is probably a bad idea... not having a good evening.

No worries.

When working on a pull request, I usually run git rebase master to bring changes from the master branch, and then commit additional changes in new commits. This allows reviewers to look at incremental changes only.

Fortunately, your pull request is reasonably small, so it's not that difficult to review it in whole.

dougal83
dougal83 commented Jan 11, 2020
View reviewed changes
Comment thread packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts
@dougal83
dougal83 requested a review from bajtos January 11, 2020 10:53
bajtos
bajtos approved these changes Jan 21, 2020
View reviewed changes

@bajtos bajtos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quickly skimmed through the changes and don't see any obvious problems.

Let's get one more approval before landing please, I'll ping @strongloop/sq-lb-apex via Slack.

Comment thread packages/openapi-v3/src/__tests__/unit/filter-schema.unit.ts Outdated
Comment thread packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts
@dougal83
dougal83 force-pushed the feat(repository-json-schema)add-title-to-filter-schemas branch from 05945db to 7a7cbd2 Compare January 21, 2020 19:48
@dougal83

dougal83 commented Jan 21, 2020

Copy link
Copy Markdown
Contributor Author

@bajtos Thanks for the feedback. How do I resolve the commit lint failure?

@bajtos

bajtos commented Jan 23, 2020

Copy link
Copy Markdown
Member

@dougal83

How do I resolve the commit lint failure

Huh, it looks like the linter does not understand that your pull request is adding a single commit only and as a result, it's linting existing commits on master. I vaguely remember that we have encountered this problem once or twice in the past, but that was years ago and I don't remember much details.

Let's try to rebase the pull request on top of the latest master to see if it helps.

@dougal83 @bajtos
feat(repository-json-schema): add title to filter schemas
1623c83 
add title property to filter schemas(filter, where, scope) in preparation for openapi schema consolidation

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@bajtos
bajtos force-pushed the feat(repository-json-schema)add-title-to-filter-schemas branch from 7a7cbd2 to 1623c83 Compare January 23, 2020 08:41
@bajtos
bajtos merged commit 6105883 into loopbackio:master Jan 23, 2020
@bajtos

bajtos commented Jan 23, 2020

Copy link
Copy Markdown
Member

Rebase fixed the problem.

The pull request is landed, thank you @dougal83 for the contribution! ❤️

@dougal83

dougal83 commented Jan 23, 2020

Copy link
Copy Markdown
Contributor Author

@bajtos Thank you for sorting it out for me!

This was referenced Jan 24, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raymondfeng raymondfeng raymondfeng approved these changes
@derdeka derdeka derdeka approved these changes
+2 more reviewers
@jannyHou jannyHou jannyHou left review comments
@bajtos bajtos bajtos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@bajtos bajtos

@dougal83 dougal83

@jannyHou jannyHou

Labels

developer-experience Issues affecting ease of use and overall experience of LB users feature Schema

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dougal83 @bajtos @raymondfeng @jannyHou @derdeka